-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: optimistic updates #2
base: main
Are you sure you want to change the base?
Conversation
@@ -24,6 +33,11 @@ export function tapState<QueryData>(callbacks: { | |||
|
|||
case 'revalidate': | |||
callbacks.onRevalidate?.(); | |||
|
|||
if (options?.optimisticUpdates) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably missing something but how does this "enable" optimistic updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is "just" a refresh?
Optimistic updates are used when data is being updated and you already want to reflect the changes to the user, without waiting for the server response.
See the docs for react-query https://react-query.tanstack.com/guides/optimistic-updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this PR, while the data was being revalidated, we didn't have a way to show any previous values.
See the before:
https://cdn.discordapp.com/attachments/962720938774921247/971513436569755688/Kapture_2022-05-04_at_16.45.17.gif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, I don't know if optimisticUpdates
is the correct name then.
What about preload
?
Could you also add a test for this please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m ok with renaming the setting! Preload sounds a lot like prefetch. But I do like your first suggestion, refresh. Maybe backgroundRefresh?
Should the setting be able to be globally set to a default?
I will add tests and update with new examples once things are decided
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, after giving it some more thought I think we can make this possible without an additional config flag.
The data should be passed to the revalidate method of tapState
.
The component code then looks like this if you want to use this future.
Notice the extra if statement because person
will be undefined the first time because it isn't in the cache.
ngOnInit(): void {
this.queryState.effect(
this.queryState.data$.pipe(
tapState({
onSuccess: (person) => {
this.model.id = person.id;
this.model.firstname = person.firstname;
this.model.lastname = person.lastname;
},
+ onRevalidate: (person) => {
+ if (person) {
+ this.model.id = person.id;
+ this.model.firstname = person.firstname;
+ this.model.lastname = person.lastname;
+ }
},
})
)
);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned that solution initially on our discord conversation, but I think that the downside of this is that you will have to wire up all your components on both onSuccess, onRevalidate and onError, etc.
What I was thinking is that we can with some configuration set some of these things up so that we get the desired output with less boiler plate need. Like shown in this PR.
Anyone who wants custom handlers for each tap, can have that as well.
but these are just thoughts. I don’t think strongly about one over the other, so I’ll leave it up to you to decide :)
this will allow trigger onSuccess during revalidation. Let me know what you think